Variable definitions validation #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

jml merged 27 commits into haskell-graphql:master from theobat:variable-definitions-validation

Jun 28, 2018

Contributor

theobat commented Jun 21, 2018 •

edited

Loading

This PR is a required step towards the resolution of #145. It improves the validation of type declaration in variable definitions. For instance it validates the fact that when a document contains $myVariable: MyVar, then the MyVar type actually exists in the schema and it's an InputType.

Remaining tasks:

test cases for the new ValidationErrors
fix current broken EndToEnd tests

PS: this is my first "real" attempt to write haskell so please, any insight would be greatly appreciated.

Edit: I added DefinesTypes instances on arguments because failing tests were simply not finding their argument types in the schema (schema in the sense of what is returned by makeSchema)

theobat added 4 commits

June 21, 2018 12:08


          feat: change VariableDefinition in validation to use Schema

d7ac50e


          feat: add doc + better errors


          fix: unused arg

112c494


          test: first solve

1c1393d

jml reviewed

View reviewed changes

Collaborator

jml left a comment

Thanks! This looks very much along the lines that we spoke about, and is great work for a first serious Haskell PR.

As requested, I've tried to provide as much feedback as I can. The things marked "Nit" are ultimately unimportant things that I cannot stop myself from caring about.

src/GraphQL/Internal/Schema.hs Outdated

               makeSchema :: ObjectTypeDefinition -> Schema
               makeSchema = Schema . getDefinedTypes
+              emptySchema :: Schema

Collaborator

jml Jun 21, 2018

Please add a doc comment. Ideally it should say what it's used for, or why you might want to use it.

src/GraphQL/Internal/Schema.hs Outdated

+              -- | Convert the given TypeDefinition to an InputTypeDefinition if it's a valid InputTypeDefinition
+              -- (because InputTypeDefinition is a subset of TypeDefinition)
+              -- see <http://facebook.github.io/graphql/June2018/#sec-Input-and-Output-Types>

Collaborator

jml Jun 21, 2018

Please put single quotes around TypeDefinition etc. This makes Haddock render them as links.

src/GraphQL/Internal/Validation.hs Outdated

@@ @@ -174,7 +181,7 @@ validateOperations schema fragments ops = do @@
                 traverse validateNode deduped
                 where
                   validateNode (operationType, AST.Node _ vars directives ss) =
-                    operationType <$> lift (validateVariableDefinitions vars)
+                    operationType <$> lift ((validateVariableDefinitions schema) vars)

Collaborator

jml Jun 21, 2018

I don't think the inner parentheses are necessary.

src/GraphQL/Internal/Validation.hs Outdated

               data VariableDefinition
                 = VariableDefinition
                   { variable :: Variable -- ^ The name of the variable
-                  , variableType :: AST.GType -- ^ The type of the variable
+                  -- , variableType :: AST.GType -- ^ The type of the variable

Collaborator

jml Jun 21, 2018

Please remove all commented out code.

src/GraphQL/Internal/Validation.hs Outdated

+              validateVariableDefinition schema (AST.VariableDefinition var varType value) =
+                case validateTypeAssertion schema var varType of
+                  Left e -> throwE e
+                  Right t -> VariableDefinition var t <$> traverse validateDefaultValue value

Collaborator

jml Jun 21, 2018

Couple of things here.

First, I think this code could be written the same way as:

do
  t <- validateTypeAssertion schema var varType
  VariableDefinition var t <$> traverse validateDefaultValue value

since that's the way the Validation monad behaves.

But, you probably don't want to use the monadic behaviour. Why? Because writing this as "first validateTypeAssertion, then validateDefaultValue" implicitly communicates that validateDefaultValue depends on validateTypeAssertion. But of course it doesn't.

From a user experience point of view, this means if there's a problem with the type assertion and a problem with the default value, they will only get the error from the type assertion. This is not ideal.

Instead, you want to use the applicative behaviour. e.g.

  VariableDefinition var <$> validateTypeAssertion schema var varType <*> traverse validateDefaultValue value

This clearly shows that the two are independent, and the Validation applicative instance will make sure both errors are included (if there happen to be errors from both).

Does this make sense?

Contributor Author

theobat Jun 23, 2018

Yes. Not sure I thoroughly grasp the entire concepts of Applicative and Monad, but as far as this example is concerned I think I understand.

src/GraphQL/Internal/Validation.hs Outdated

+                | tname == getName GID = Right (BuiltinInputType GID)
+                | otherwise = Left (VariableTypeNotFound var tname)
+              -- | simple translation between ast annotation types and schema annotation types

Collaborator

jml Jun 21, 2018

Nit: capital first letter, capitalize "AST" and end with a full stop.

src/GraphQL/Internal/Validation.hs Outdated

+              astAnnotationToSchemaAnnotation gtype schematn =
+                case gtype of
+                  AST.TypeNamed _ -> TypeNamed schematn
+                  AST.TypeList (AST.ListType asttn) -> astAnnotationToSchemaAnnotation asttn schematn

Collaborator

jml Jun 21, 2018

schemaTypeName, astTypeName

src/GraphQL/Internal/Validation.hs Outdated

+              astAnnotationToSchemaAnnotation gtype schematn =
+                case gtype of
+                  AST.TypeNamed _ -> TypeNamed schematn
+                  AST.TypeList (AST.ListType asttn) -> astAnnotationToSchemaAnnotation asttn schematn

Collaborator

jml Jun 21, 2018

This looks wrong. The annotated type of "list of int" should be "list of int", not "int".

src/GraphQL/Internal/Validation.hs Outdated

+              -- | validate a variable type which has no type definition (either builtin or not in the schema)
+              validateVariableTypeBuiltin :: Variable -> Name -> Either ValidationError InputType
+              validateVariableTypeBuiltin var tname

Collaborator

jml Jun 21, 2018

Why does this take a Variable?

Contributor Author

theobat Jun 25, 2018

It takes a variable name because it enables us to give the variable name in the error message (if the given type is not a builtin type, which is the error message if we give a random type name for instance). I do believe this is valuable.

tests/ValidationTests.hs Outdated

		@@ -27,11 +29,11 @@ someName = "name"

		dog :: Name
		dog = "dog"

Collaborator

jml Jun 21, 2018

Nit: Please don't add unnecessary whitespace.

theobat added 21 commits

June 23, 2018 12:35


          fix: emptySchema doc

83c3c5e


          fix: single quotes on getInputTypeDefinition

471a658


          fix: pointless parentheses

dc1ea69


          fix: remove commented code

dd9d4fe


          fix: non idiomatic functions

38382a0


          fix: astAnnotationToSchemaAnnotation

07a242f


          chores: cosmetic variable names

e3106fa


          feat: adding DefinesTypes instance for arguments

d8f2040


          chores: removing redundant imports

79aeb22


          chores: unused import && useless bracket

c48296f


          fix: haddock generation error

11139e2


          test: non-existing type in variable definition

ae3c668


          test: unused variable definition + others expected to fail

2e3a5ed


          test: removing invalid test

8c9d7fb


          test: schema & ast improvements


          tests: end-to-end annotation && non-null

f8e7098


          fix: unused variable

59e532e


          test: complex inline argument

205a4ab


          test: ast && validation

2ee2296


          fix: astAnnotationToSchemaAnnotation following test spec

aa7af4b


          test: some formatError tests

03b43ba

Contributor Author

theobat commented Jun 28, 2018

@jml, Now I think I'm done here, for some reason the tests are marked as failed on CircleCI, but there isn't any error at any stage of the pipeline and the coverage increased everywhere:

#!/bin/bash -eo pipefail
STACK_YAML=stack-8.2.yaml ./scripts/hpc-ratchet

alternatives: BETTER (175 missing => 161 missing)
booleans: OK
expressions: BETTER (1494 missing => 1416 missing)
local_decls: BETTER (15 missing => 14 missing)
top_level_decls: BETTER (685 missing => 669 missing)
Exited with code 2

I'm still open to other changes though if things are not right yet.

Collaborator

jml commented Jun 28, 2018 •

edited

Loading

@theobat Thanks, I'll take a look now.

To fix the coverage error, you need to update the numbers in the script to match the new, better numbers: https://github.com/haskell-graphql/graphql-api/blob/master/scripts/hpc-ratchet#L37-L43

(Edit: correct link)

jml approved these changes

View reviewed changes

Collaborator

jml left a comment

This looks great, thanks! I'm super excited to see the improved test coverage.

One minor comment.

tests/EndToEndTests.hs Outdated

+                            , "errors" .=
+                              [
+                                object
+                                -- TODO: cf previous test case

Collaborator

jml Jun 28, 2018

Not sure I could do this todo. Please either do it or expand on it so that someone who's not you could do it.

Contributor Author

theobat Jun 28, 2018 •

edited

Loading

So this is just a reference to the one above that I did not write.

-- TODO: This error message is pretty bad. We should define
-- a typeclass for client-friendly "Show" (separate from
-- actual Show which remains extremely useful for debugging)
-- and use that when including values in error messages.

Now, while I understand both the idea and how to do it (I think...), it seems like a lot of work for this PR. Since both error messages are pretty close, I thought it would be better to add the TODO reference to the other one, but now that I'm reading it on github, it seems rather unhelpful. I'm going to simply remove it and let the original TODO as it is


          chores: hpc tests number increased

8ae8810


          fix: unhelpful error message

1fb7249

Contributor Author

theobat commented Jun 28, 2018

@jml There we are ! Thanks for your reviews. As a side note, it seems like every guard containing an otherwise statement is counted as "always true" by hpc which means increasing the number of otherwise might force us to decrease coverage..

57% guards (11/19), 6 always True, 2 unevaluated

Next step is a PR with the actual Aeson.Object to Value translation. I won't address it before some time though.

Collaborator

jml commented Jun 28, 2018

As a side note, it seems like every guard containing an otherwise statement is counted as "always true" by hpc

Yeah, that's something I really would like to fix in hpc (or with some kind of tooling), rather than compromising on coverage. The Haskell testing ecosystem needs to get better.

Collaborator

jml commented Jun 28, 2018

Thanks!

jml merged commit 377c332 into haskell-graphql:master

theobat deleted the variable-definitions-validation branch

June 28, 2018 14:55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet